Repository navigation
fix(v0.2): transport-aware liveness at 5 callers of isPaneAlive (complements #1167) - #1186
Conversation
Site 1 findDeadResumable: safe — provider filter excludes SDK (documented).
Site 2 rejectDuplicateRole: dispatch via resolveWorkerLivenessByTransport so
live SDK/omni/inline duplicates are detected instead of being silently
cleaned. Session-recycle check gated on tmux paneId (synthetic paneIds
never collide with a recycled pane).
Site 3 resolveSpawnIdentity: widen isAliveFn signature to {id, paneId};
default routes through the shared helper. Prevents a live SDK canonical
from being wrongly re-canonicalized and clobbering its session UUID via
ON CONFLICT UPDATE. Updates test stubs in agents.test.ts and
tui-spawn-dx.integration.test.ts to match signature.
Complements #1167.
Site 4 checkWorkerAlive (scheduler-daemon.ts): documented safe — runs.worker_id only carries real PIDs or a UUID daemonId fallback from handleTrigger; never a tmux pane or synthetic id. SDK agents live in agents/executors, not runs. Site 5 runAgentRecoveryPass (scheduler-daemon.ts): documented safe — claudeSessionId filter excludes SDK agents (SDK lacks a Claude-CLI JSONL). Site 6 isAgentAlive (team-auto-spawn.ts): dispatch via shared helper so inbox-watcher correctly routes messages to live SDK recipients instead of treating them as dead and dropping/misrouting. Site 7 executeAutoSpawn (hooks/handlers/auto-spawn.ts): dispatch via shared helper so the PreToolUse SendMessage handler stops triggering duplicate spawns on every message to a live SDK recipient. Complements #1167.
Mirrors auto-resume-zombie-cap.test.ts:229-251 "synthetic paneIds" pattern: - 6 unit tests for resolveWorkerLivenessByTransport dispatch (tmux %N, sdk/inline/'', dead synthetic, dead tmux pane never falls through). - 4 DB-backed integration tests: isAgentAlive live SDK, isAgentAlive dead SDK, resolveSpawnIdentity live SDK → parallel, resolveSpawnIdentity dead SDK → canonical. Sites 2 and 7 are covered transitively — both callers pass their registry row straight to the shared helper, so the dispatch tests pin their contract.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements transport-aware liveness checks to correctly handle non-tmux agents (SDK, omni, and inline), preventing them from being misreported as dead. By introducing the resolveWorkerLivenessByTransport helper, the system now dispatches liveness checks based on the paneId format, querying the executor registry for synthetic IDs. Review feedback focuses on enhancing type safety by making paneId nullable in the new helper and associated test utilities, as well as refactoring the tmux pane ID regex into a shared utility to eliminate code duplication across the codebase.
| export async function resolveWorkerLivenessByTransport( | ||
| worker: { id: string; paneId: string }, |
There was a problem hiding this comment.
The paneId property in the worker parameter should be nullable to match the Agent type in the registry, where paneId can be null or undefined (e.g., for agents that haven't been assigned a transport yet). Updating the type here will prevent potential TypeScript errors at call sites like rejectDuplicateRole in agents.ts or autoSpawn in auto-spawn.ts.
export async function resolveWorkerLivenessByTransport(
worker: { id: string; paneId: string | null | undefined },| isExecutorAliveFn?: (agentId: string) => Promise<boolean>; | ||
| }, | ||
| ): Promise<boolean> { | ||
| if (/^%\d+$/.test(worker.paneId)) { |
There was a problem hiding this comment.
The regex /^%\d+$/ is used to identify tmux pane IDs. Since paneId can be nullable, it's safer to check for its existence before testing. Additionally, this regex is duplicated in src/term-commands/agents.ts. Consider defining a shared utility or constant for this check to improve maintainability.
if (worker.paneId && /^%\d+$/.test(worker.paneId)) {| // completely different session now. Verify the pane is still in the | ||
| // expected session before blocking. Only applies to real tmux panes; | ||
| // synthetic paneIds (sdk/inline/'') never collide with a recycled pane. | ||
| if (alive && w.session && /^%\d+$/.test(w.paneId)) { |
There was a problem hiding this comment.
This regex check for tmux pane IDs is duplicated from resolveWorkerLivenessByTransport in executor-registry.ts. While the logic is correct (session verification only applies to tmux), using a shared utility function like isTmuxPaneId(paneId) would be cleaner and less error-prone if the format ever changes.
| function alivePanes(...alive: string[]): (agent: { id: string; paneId: string }) => Promise<boolean> { | ||
| const set = new Set(alive); | ||
| return async (paneId: string) => set.has(paneId); | ||
| return async (agent: { id: string; paneId: string }) => set.has(agent.paneId); |
There was a problem hiding this comment.
The signature of alivePanes should be updated to allow paneId to be nullable, matching the updated signature of resolveWorkerLivenessByTransport and the underlying Agent type.
function alivePanes(...alive: string[]): (agent: { id: string; paneId: string | null | undefined }) => Promise<boolean> {
const set = new Set(alive);
return async (agent: { id: string; paneId: string | null | undefined }) => !!agent.paneId && set.has(agent.paneId);![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83a5b572f7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const w of existing) { | ||
| if (w.role === role && w.team === team) { | ||
| const alive = await isPaneAlive(w.paneId); | ||
| const alive = await executorRegistry.resolveWorkerLivenessByTransport(w); |
There was a problem hiding this comment.
Switching duplicate-role detection to resolveWorkerLivenessByTransport makes SDK/inline rows live-or-dead depend entirely on isExecutorAlive, which currently considers spawning/running live with no freshness check. If a non-tmux worker dies ungracefully (for example, after launchSdkSpawn creates a spawning executor but before it reaches the cleanup path), this check returns true and rejectDuplicateRole exits instead of unregistering the stale row, so the user cannot respawn that role. This regression is specific to synthetic pane IDs because tmux rows still get authoritative pane liveness.
Useful? React with 👍 / 👎.
Summary
Complements #1167 and mirrors the
%\d+-regex-guard pattern from #1181'scountActiveWorkers. Adds a shared helperresolveWorkerLivenessByTransportin
executor-registry.tsthat dispatches on paneId shape:%N) →isPaneAlive(paneId)(authoritative for tmux)sdk,inline,'') →isExecutorAlive(agentId)whichconsults
executors.state— the live signal for non-tmux transportsApplies the helper at 4 of the 7 parallel-pattern sites audited by the tracer
and widens one signature at the 5th site. Two sites (1 + 5) are already safe
due to upstream filters and get inline comments instead.
Site-by-site
findDeadResumable(agents.ts)provider === 'claude'filter)rejectDuplicateRole(agents.ts)isPaneAlive('sdk')→ false → silently unregisters live SDK duplicateresolveSpawnIdentity(agents.ts)(paneId)→ live SDK canonical wrongly re-canonicalized → ON CONFLICT UPDATE clobbers session UUID{ id, paneId }default = shared helper; live SDK canonical routes to parallel, row preservedcheckWorkerAlive(scheduler-daemon.ts)runs.worker_idis only ever a PID or UUIDdaemonIdfromhandleTrigger(never pane/synthetic); SDK agents live inagents/executors, notruns. Documented.runAgentRecoveryPass(scheduler-daemon.ts)claudeSessionIdfilter)isAgentAlive(team-auto-spawn.ts)executeAutoSpawn(hooks/handlers/auto-spawn.ts)Files touched
src/lib/executor-registry.ts— helpersrc/term-commands/agents.ts— sites 1, 2, 3src/lib/scheduler-daemon.ts— sites 4 + 5 documentationsrc/lib/team-auto-spawn.ts— site 6src/hooks/handlers/auto-spawn.ts— site 7src/__tests__/tui-spawn-dx.integration.test.ts— stub update for new signaturesrc/term-commands/agents.test.ts— stub comment updatesrc/lib/__tests__/transport-aware-liveness.test.ts— new, 10 regression testsTest plan
bun run check→ 2625 pass / 0 failisAgentAlivelive/dead SDK,resolveSpawnIdentitylive SDK → parallel, dead SDK → canonicalauto-resume-zombie-cap.test.ts:229-251 "synthetic paneIds"still passesScope rules honored
buildWorkerStatusMap,resolveWorkerLiveness) except to reference them in docs.countActiveWorkersexcept to document the shared pattern.isPaneAlivecallers outside the tracer's 7-site list.executorstable.🤖 Generated with Claude Code